Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow setting ApiCompatExcludeNonBrowsable in the csproj #5465

Closed
wants to merge 2 commits into from
Closed

Allow setting ApiCompatExcludeNonBrowsable in the csproj #5465

wants to merge 2 commits into from

Conversation

lbargaoanu
Copy link
Contributor

@lbargaoanu lbargaoanu commented May 13, 2020 •
edited
Loading

@dougbu Perhaps I'm supposed to use AdditionalApiCompatOptions. Then I guess I should close this PR and simply add smth in the readme about the new option.

@lbargaoanu lbargaoanu changed the title ApiCompat: Allow setting ApiCompatExcludeNonBrowsable in the csproj Allow setting ApiCompatExcludeNonBrowsable in the csproj May 13, 2020
@dougbu
Copy link
Member

dougbu commented May 13, 2020

@lbargaoanu I defer to others e.g. @riarenas, @safern or @ericstj on the utility of adding single-option properties now that we have $(AdditionalApiCompatOptions). I lean toward using $(AdditionalApiCompatOptions) but am not fully committed to that position 😺

Either way, I should have updated src/Microsoft.DotNet.ApiCompat/README.md in #4585 to include $(AdditionalApiCompatOptions). If we move forward with this PR, please add some words about that one too.

@safern
Copy link
Member

safern commented May 13, 2020

I also agree that we should instead just use AdditionalApiCompatOptions, it seems kind of overkill to try and add a boolean flag for all ApiCompat options into the targets to just translate them to command line args.

but I also agree that we should uptate the docs to mention AdditionalApiCompatOptions.

@lbargaoanu lbargaoanu closed this May 13, 2020
@lbargaoanu lbargaoanu deleted the patch-1 branch May 13, 2020 19:06
@lbargaoanu
Copy link
Contributor Author

Replaced by #5471.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ViktorHofer ViktorHofer Awaiting requested review from ViktorHofer

@safern safern Awaiting requested review from safern

@ericstj ericstj Awaiting requested review from ericstj

@riarenas riarenas Awaiting requested review from riarenas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lbargaoanu @dougbu @safern